-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add object actions - presigned URL, download and preview #1604
Add object actions - presigned URL, download and preview #1604
Conversation
@@ -91,6 +92,7 @@ | |||
"js-base64": "^2.1.9", | |||
"js-yaml": "^3.13.1", | |||
"lodash-es": "^4.17.21", | |||
"luxon": "^3.3.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reason why we need this: https://stackoverflow.com/a/1214753
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Luxon is a modern replacement for deprecated Moment.js
const CreatedOnSkeleton: React.FC<{}> = () => ( | ||
<Skeleton width="25%" height="15%" /> | ||
); | ||
|
||
const CreatedOn: React.FC<{ bucketName: string }> = ({ bucketName }) => { | ||
const { t } = useCustomTranslation(); | ||
|
||
const { noobaaS3 } = React.useContext(NoobaaS3Context); | ||
const { data, error, isLoading } = useSWR(LIST_BUCKET, () => | ||
noobaaS3.listBuckets() | ||
); | ||
|
||
const bucketCreatedOn = | ||
!isLoading && !error | ||
? data?.Buckets?.find((bucket) => bucket?.Name === bucketName) | ||
?.CreationDate | ||
: null; | ||
|
||
return isLoading ? ( | ||
<CreatedOnSkeleton /> | ||
) : ( | ||
<h4 className="text-muted"> | ||
{t('Created on: ') + bucketCreatedOn?.toString()} | ||
</h4> | ||
); | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can not be determined that easily, decided to remove it instead...
{ | ||
title: t('Copy Object URL'), | ||
onClick: () => undefined, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this... we can not determine this...
004d58f
to
050d868
Compare
050d868
to
6b17549
Compare
9e945f5
to
2a2cda6
Compare
2a2cda6
to
b98f355
Compare
) => Promise<string>; | ||
|
||
export type DownloadAndPreviewState = { | ||
isdownloading: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isdownloading: boolean; | |
isDownloading: boolean; |
// open the object URL in a new browser tab | ||
window.open(objectURL, '_blank'); | ||
} catch (err) { | ||
// eslint-disable-next-line no-console |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can consider to allow error
console method in order to avoid adding eslint-disable comments...
If we go ahead this can be done in a cleanup PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense to me !!
b98f355
to
e409855
Compare
e409855
to
607cd34
Compare
export const numberInputOnChange = | ||
(min: number, max: number, onChange: (value: number) => void) => | ||
(input: React.FormEvent<HTMLInputElement>): void => { | ||
const inputValue = +(input.target as HTMLInputElement)?.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this min-max comparision? is this for the corner case of entering the value directly?
I am assuming NumberInput won't allow to increase or decrease values less than or greater than min max value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be used when u will type/enter the number directly into the input...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
||
return ( | ||
<FormGroup label={t('Expires after')} fieldId="expires-after"> | ||
<NumberInput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is RequestSizeInput can be used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, maybe we can utilise it with some refactoring... but will take it up in a followup PR...
<FormGroup label={t('Share link')} fieldId="share-link"> | ||
<div className="pf-v5-u-text-align-right text-muted"> | ||
<b>{t('Valid until: ')}</b> | ||
{urlDetails.current.validUntil}{' '} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{urlDetails.current.validUntil}{' '} | |
{urlDetails.current.validUntil} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added by linter... AFAIR I didn't add this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
LGTM |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alfonsomthd, SanjalKatiyar The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
abb8239
into
red-hat-storage:master
https://issues.redhat.com/browse/RHSTOR-6509
https://issues.redhat.com/browse/RHSTOR-6507
https://issues.redhat.com/browse/RHSTOR-6510